Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kubernetes: Add a secure config that can use externally created certs #27921

Merged
merged 1 commit into from
Sep 20, 2018

Conversation

a-robinson
Copy link
Contributor

For Kubernetes clusters that don't sign certificates properly, such as
EKS as discovered in a recent forum post:
https://forum.cockroachlabs.com/t/secure-cockroachdb-cluster-on-aws-eks/1824

Release note: None


It's arguable that this isn't worth checking in. I'm not too attached to it if folks don't think we should, I just needed to publish it somewhere for the original poster of https://forum.cockroachlabs.com/t/secure-cockroachdb-cluster-on-aws-eks/1824 to be able to use it.

Touches #24527

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@dkeightley
Copy link

dkeightley commented Jul 26, 2018

I was able to reproduce the issue in the forum post, could this issue be related to: kubernetes/kubernetes#44151 (comment)

Checking the workflow in EKS, I can see in the init container the addresses (SANs) are in the request..

+ /request-cert '-namespace=default' '-certs-dir=/cockroach-certs' '-type=node' '-addresses=localhost,127.0.0.1,cockroachdb-0.cockroachdb.default.svc.cluster.local,cockroachdb-0.cockroachdb,cockroachdb-public,cockroachdb-public.default.svc.cluster.local' '-symlink-ca-from=/var/run/secrets/kubernetes.io/serviceaccount/ca.crt'

And these appear in the resulting CSR..

            X509v3 Subject Alternative Name: 
                DNS:localhost, DNS:cockroachdb-0.cockroachdb.default.svc.cluster.local, DNS:cockroachdb-0.cockroachdb, DNS:cockroachdb-public, DNS:cockroachdb-public.default.svc.cluster.local, IP Address:127.0.0.1

In the approved certificate no SAN is mentioned, this can be explained as a custom certificate signer is used in EKS (see below) which ignores SANs.

@mberhault
Copy link
Contributor

@a-robinson: I think it's worth checking in, we seem to have run into cert problems on k8s a few times, any known solutions are good.

@a-robinson
Copy link
Contributor Author

Hi @dkeightley!

I was able to reproduce the issue in the forum post, could this issue be related to: kubernetes/kubernetes#44151 (comment)

That doesn't look related. The problem there is that the common name is ignored when SANs are present. The problem here is that the signed certificates don't include the requested SANs.

In the approved certificate no SAN is mentioned, as such this doesn't appear to be EKS specific but perhaps an integration with the Kubernetes certificate management?

It certainly could be, but I haven't seen any similar problems on Minikube, GKE, and the on-prem deployments of the customers I've worked with. The docs advise that the kube-controller-manager must be provided with a couple command-line arguments in order to be able to sign certificates, but make no mention of scenarios in which certificates are signed but not all fields are propagated to them. I suspect that EKS may be using something other than the kube-controller-manager implementation to sign certs and its implementation ignores SANs, but that's pure conjecture at this point.

I'm not gonna be able to look into it within the next couple weeks either way, so if you learn anything I'd love to know what you find.

@a-robinson
Copy link
Contributor Author

ping @bobvawter

@dkeightley
Copy link

In EKS, only certificates for the Kubelet are intended to be signed, as such SANs are ignored and the Common Name (CN=kubernetes) does not match the hostname for CockroachDB.

To resolve this an alternative CA can be used, programatically this could perhaps be achieved with an init container and easyrsa, with the resulting cert stored/retrieved from Secrets.

@a-robinson
Copy link
Contributor Author

I've moved this from cloud/kubernetes/experimental to cloud/kubernetes/extras. Does that naming seem reasonable to you, @mberhault? It's about time I get this in.

@mberhault
Copy link
Contributor

LGTM. Do we need a blurb to show how to do the same for clients running in the same cluster?

For Kubernetes clusters that don't sign certificates properly, such as
EKS as discovered in a recent forum post:
https://forum.cockroachlabs.com/t/secure-cockroachdb-cluster-on-aws-eks/1824

Release note (general change): A new Kubernetes configuration has been
added that shows how to use certificates generated outside of the
Kubernetes cluster.
@a-robinson
Copy link
Contributor Author

Yeah, good call. Done. I ended up moving the files into a more clearly named directory to strongly associate them with each other and avoid a future where we have a bunch of random garbage in an "extras" directory. Any other specialized configs can have their own directories.

@mberhault
Copy link
Contributor

LGTM, nice directory name and thanks for the client config. CCing #30188 which discusses using external certificates (though in that case it's client certificates).

@a-robinson
Copy link
Contributor Author

bors r+

craig bot pushed a commit that referenced this pull request Sep 20, 2018
27921: kubernetes: Add a secure config that can use externally created certs r=a-robinson a=a-robinson

For Kubernetes clusters that don't sign certificates properly, such as
EKS as discovered in a recent forum post:
https://forum.cockroachlabs.com/t/secure-cockroachdb-cluster-on-aws-eks/1824

Release note: None

--------------------

It's arguable that this isn't worth checking in. I'm not too attached to it if folks don't think we should, I just needed to publish it somewhere for the original poster of https://forum.cockroachlabs.com/t/secure-cockroachdb-cluster-on-aws-eks/1824 to be able to use it.

Touches #24527

30256: storage: Transfer lease to least-loaded store when rebalancing replicas r=a-robinson a=a-robinson

This logic was totally busted before due to indexing into the wrong
replicas slice, meaning we would often transfer the lease to the wrong
store.

Release note: None

Co-authored-by: Alex Robinson <[email protected]>
@craig
Copy link
Contributor

craig bot commented Sep 20, 2018

Build succeeded

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants